117 delay option for new set of key signing #118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JaCoCo code coverage report - scala:2.12.17
|
dk1844
reviewed
Feb 6, 2025
Comment on lines
39
to
40
| keyPhaseOutTime: Option[FiniteDuration], | ||
| keyLayOverTime: Option[FiniteDuration] |
There was a problem hiding this comment.
Suggested change
| keyPhaseOutTime: Option[FiniteDuration], | |
| keyLayOverTime: Option[FiniteDuration] | |
| keyLayOverTime: Option[FiniteDuration], | |
| keyPhaseOutTime: Option[FiniteDuration] |
Same reason as above.
| @@ -79,7 +80,15 @@ case class AwsSecretsManagerKeyConfig( | |||
| } | |||
| } | |||
|
|
|||
| (currentKeyPair, previousKeyPair) | |||
| previousKeyPair.fold {(currentKeyPair, previousKeyPair)} { pk => | |||
| val exp = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _)) | |||
There was a problem hiding this comment.
Suggested change
| val exp = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _)) | |
| val layoverIsActive = keyLayOverTime.exists(!isExpired(currentSecrets.createTime, _)) |
I think better names denoting more closely what is meant would be welcome. Same with
val exp = keyPhaseOutTime.exists(isExpired(currentSecrets.createTime, _))(perhaps rename to val phaseOutIsActive)?
api/src/main/scala/za/co/absa/loginsvc/rest/config/jwt/AwsSecretsManagerKeyConfig.scala
Outdated
Show resolved
Hide resolved
api/src/test/scala/za/co/absa/loginsvc/rest/service/jwt/JWTServiceTest.scala
Outdated
Show resolved
Hide resolved
dk1844
reviewed
Feb 12, 2025
README.md
Outdated
Comment on lines
222
to
223
| t=15-29m: layover is over: new key from after rotation is used for signing. Both public keys available from public-keys endpoint. | ||
| t=30m+: phase-out happens: new key from after rotation is used for signing. Old Key is no longer available from public-keys endpoint. |
There was a problem hiding this comment.
Suggested change
| t=15-29m: layover is over: new key from after rotation is used for signing. Both public keys available from public-keys endpoint. | |
| t=30m+: phase-out happens: new key from after rotation is used for signing. Old Key is no longer available from public-keys endpoint. | |
| t=15-44m: layover is over: new key from after rotation is used for signing. Both public keys available from public-keys endpoint. | |
| t=45m+: phase-out happens: new key from after rotation is used for signing. Old Key is no longer available from public-keys endpoint. |
Since now you have made phase-out window to be counted after the layover is concluded.
README.md
Outdated
Comment on lines
264
to
265
| t=15-29m: layover is over: new key from after rotation is used for signing. Both public keys available from public-keys endpoint. | ||
| t=30m+: phase-out happens: new key from after rotation is used for signing. Old Key is no longer available from public-keys endpoint. |
There was a problem hiding this comment.
Suggested change
| t=15-29m: layover is over: new key from after rotation is used for signing. Both public keys available from public-keys endpoint. | |
| t=30m+: phase-out happens: new key from after rotation is used for signing. Old Key is no longer available from public-keys endpoint. | |
| t=15-44m: layover is over: new key from after rotation is used for signing. Both public keys available from public-keys endpoint. | |
| t=45m+: phase-out happens: new key from after rotation is used for signing. Old Key is no longer available from public-keys endpoint. |
Same as above, I think
dk1844
reviewed
Feb 12, 2025
api/src/test/scala/za/co/absa/loginsvc/rest/config/jwt/AwsSecretsManagerKeyConfigTest.scala
Outdated
Show resolved
Hide resolved
api/src/main/scala/za/co/absa/loginsvc/utils/AwsSecretsUtils.scala
Outdated
Show resolved
Hide resolved
api/src/main/scala/za/co/absa/loginsvc/rest/config/jwt/AwsSecretsManagerKeyConfig.scala
Outdated
Show resolved
Hide resolved
dk1844
approved these changes
Feb 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Release Notes:
closes #117